add AnonConst as potential parent_item #71477

lcnr · 2020-04-23T18:08:43Z

This prevents some help messages in const contexts, due to

rust/src/librustc_typeck/check/demand.rs

Lines 597 to 601 in 413a129

    
           if self.tcx.hir().is_const_context(expr.hir_id) { 
        
               // Shouldn't suggest `.into()` on `const`s. 
        
               // FIXME(estebank): modify once we decide to suggest `as` casts 
        
               return false; 
        
           }

considering that the check in is_const_context was previously unreachable,
this is actually a bugfix 😆

rust/src/librustc_middle/hir/map/mod.rs

Line 563 in 413a129

| Node::AnonConst(_) => true,

r? @eddyb cc @estebank

lcnr · 2020-04-23T18:10:01Z

src/test/ui/repeat_count.stderr

-   |
-help: you can convert an `isize` to `usize` and panic if the converted value wouldn't fit
-   |
-LL |     let f = [0; (-4_isize).try_into().unwrap()];


This help message was incorrect.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=3e0d446869bd89792a113dbd8f140f15

estebank · 2020-04-23T18:51:20Z

It'd be nice if we changed check_for_cast to still emit a suggestion for literal suffix changes. Would you mind doing that in a separate PR or filing a ticket after this is merged?

lcnr · 2020-04-23T19:08:17Z

Can implement it in a separate PR 👍

eddyb · 2020-04-30T13:56:24Z

I don't think this is the right approach, get_parent_item should barely exist itself, what's the actual ICE being fixed here? What's calling get_parent_item on something inside AnonConst and hoping to see the body owner? Because that's what it should be looking for, not "parent item".

lcnr · 2020-04-30T14:36:32Z

This PR is needed for https://github.com/rust-lang/rust/blob/4b575996168edbc5b7ad46229f5ed3bfee35484c/src/test/ui/const-generics/type-dependent/const-arg-in-const-arg.rs #70 (#71154 uses it to get the correct typeck_tables)

get_parent_item is also used in is_const_context to get the body owner of an anon cost.

rust/src/librustc_middle/hir/map/mod.rs

Line 564 in bf45975

let parent_id = self.get_parent_item(hir_id);

which currently results in wrong diagnostics in

rust/src/librustc_typeck/check/demand.rs

Lines 597 to 601 in 413a129

    
           if self.tcx.hir().is_const_context(expr.hir_id) { 
        
               // Shouldn't suggest `.into()` on `const`s. 
        
               // FIXME(estebank): modify once we decide to suggest `as` casts 
        
               return false; 
        
           }

to get the body owner of an anon cost.

What do you want to use instead?

eddyb · 2020-05-01T05:55:22Z

You want to loop through the parents with get_parent_node and stop when maybe_body_owned_by returns Some. That is the enclosing body owner, which is what you want.

If you want to be more efficient you could probably add an enclosing_body_owner(_def_id) which is like body_owner(_def_id) but does the loop I described above, stopping at a node for which is_body_owner returns true.

cc @ecstatic-morse @oli-obk @wesleywiser on is_const_context, I'm suspicious of its correctness and uses (it probably wants to be unified with whatever MIR const-checking uses)

lcnr · 2020-05-01T21:56:27Z

I now use the following in #71154:

    pub fn enclosing_body_owner(&self, hir_id: HirId) -> HirId {
        for (parent, _) in self.parent_iter(hir_id) {
            if let Some(body) = self.maybe_body_owned_by(parent) {
                return self.body_owner(body);
            }
        }

        bug!("no `enclosing_body_owner` for hir_id `{}`", hir_id);
    }

This means that this PR now only fixes incorrect suggestions afaict. Don't fully understand when get_parent_node is appropriate yet, so I don't know how to proceed here.

lcnr · 2020-05-19T21:11:22Z

Closing this for now as this is not the correct approach.

Will try and make a more general fix for is_const_context in the near future.

@estebank

Fix `is_const_context`, update `check_for_cast` A better version of rust-lang#71477 Adds `fn enclosing_body_owner` and uses it in `is_const_context`. `is_const_context` now uses the same mechanism as `mir_const_qualif` as it was previously incorrect. Renames `is_const_context` to `is_inside_const_context`. I also updated `check_for_cast` in the second commit, so r? @estebank (I removed one lvl of indentation, so it might be easier to review by hiding whitespace changes)

@estebank

Fix `is_const_context`, update `check_for_cast` A better version of rust-lang#71477 Adds `fn enclosing_body_owner` and uses it in `is_const_context`. `is_const_context` now uses the same mechanism as `mir_const_qualif` as it was previously incorrect. Renames `is_const_context` to `is_inside_const_context`. I also updated `check_for_cast` in the second commit, so r? @estebank (I removed one lvl of indentation, so it might be easier to review by hiding whitespace changes)

rust-highfive assigned eddyb Apr 23, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2020

lcnr commented Apr 23, 2020

View reviewed changes

lcnr added 2 commits April 23, 2020 20:11

add AnonConst as potential parent_item

2b9b78a

update test output

9421048

lcnr force-pushed the get_parent_item branch from 85195ba to 9421048 Compare April 23, 2020 18:11

lcnr mentioned this pull request Apr 23, 2020

Support const args in type dependent paths #71154

Closed

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2020

joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2020

lcnr closed this May 19, 2020

lcnr deleted the get_parent_item branch May 19, 2020 21:14

lcnr mentioned this pull request May 20, 2020

Fix is_const_context, update check_for_cast #72380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add AnonConst as potential parent_item #71477

add AnonConst as potential parent_item #71477

lcnr commented Apr 23, 2020 •

edited

Loading

lcnr Apr 23, 2020 •

edited

Loading

lcnr Apr 23, 2020

estebank commented Apr 23, 2020

lcnr commented Apr 23, 2020

eddyb commented Apr 30, 2020

lcnr commented Apr 30, 2020

eddyb commented May 1, 2020

lcnr commented May 1, 2020

lcnr commented May 19, 2020

	if self.tcx.hir().is_const_context(expr.hir_id) {
	// Shouldn't suggest `.into()` on `const`s.
	// FIXME(estebank): modify once we decide to suggest `as` casts
	return false;
	}

add AnonConst as potential parent_item #71477

add AnonConst as potential parent_item #71477

Conversation

lcnr commented Apr 23, 2020 • edited Loading

lcnr Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

lcnr Apr 23, 2020

Choose a reason for hiding this comment

estebank commented Apr 23, 2020

lcnr commented Apr 23, 2020

eddyb commented Apr 30, 2020

lcnr commented Apr 30, 2020

eddyb commented May 1, 2020

lcnr commented May 1, 2020

lcnr commented May 19, 2020

lcnr commented Apr 23, 2020 •

edited

Loading

lcnr Apr 23, 2020 •

edited

Loading